Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nbconvert: Jinjaless exporter base #4175

Merged
merged 15 commits into from Sep 18, 2013
Merged

Conversation

jdfreder
Copy link
Member

@jdfreder jdfreder commented Sep 5, 2013

@Carreau said he would be away for a week and gave me permission to rebase his PR #3747 .

@@ -100,14 +100,14 @@ def export(exporter, nb, **kw):
#Check arguments
if exporter is None:
raise TypeError("Exporter is None")
elif not isinstance(exporter, Exporter) and not issubclass(exporter, Exporter):
elif not isinstance(exporter, TemplateExporter) and not issubclass(exporter, TemplateExporter):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still be Exporter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would be better, good catch

@jdfreder
Copy link
Member Author

jdfreder commented Sep 8, 2013

@Carreau review?

@Carreau
Copy link
Member

Carreau commented Sep 8, 2013

Yes, slowly coming back from holidays, and caching up, but as always PhD first, and as my boss is back too, getting up to speed is not easy.

@Carreau
Copy link
Member

Carreau commented Sep 12, 2013

Need a rebase that I tried to force push on #3747 but github see the wrong commits after I reopened the PR...

@jdfreder
Copy link
Member Author

@Carreau , rebased

@Carreau
Copy link
Member

Carreau commented Sep 13, 2013

+1

@jdfreder
Copy link
Member Author

Sweet, unless there are objections, I'll merge sometime tomorrow, @minrk ?

@minrk
Copy link
Member

minrk commented Sep 13, 2013

@jdfreder do not merge yourself, let someone else merge.

@jdfreder
Copy link
Member Author

let someone else merge.

If you could, that would be great

default_template = Unicode(u'')
template = Any()
environment = Any()
# finish the docstring
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has a todo mark for finishing the docstring, do you want to finish it?

@minrk
Copy link
Member

minrk commented Sep 13, 2013

Looks like the rebase didn't quite go as planned - there look to be a few find/replace errors and remaining calls to transform. Also don't forget to add at least a simple exercise test that goes through the motions of the nb2nb Exporter.

@jdfreder
Copy link
Member Author

Looks like the rebase didn't quite go as planned

Apparently, gah! 😦 I wonder if that happened the second rebase around? It didn't ask for my intervention, but it did complain about some files.

Anyways, I addressed your comments, thanks, and I added a few small tests.

minrk added a commit that referenced this pull request Sep 18, 2013
add base Exporter class above TemplateExporter
@minrk minrk merged commit 197a4bf into ipython:master Sep 18, 2013
@jdfreder jdfreder deleted the exporterbase branch March 10, 2014 18:42
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
add base Exporter class above TemplateExporter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants